feat: Shows an alert in container sync if the only change is a local override to a text component [FC-0097] by ChrisChV · Pull Request #2516 · openedx/frontend-app-authoring

ChrisChV · 2025-10-07T00:42:43Z

Description

Implements the alert described in Child text override sync handling #2438 (comment)
Which user roles will this change impact? "Course Author".

Supporting information

Github issue: Child text override sync handling #2438 (comment)
Depends on: feat: Multiple updates to handle children upstream info [FC-0097] openedx-platform#37433
Internal ticket: FAL-4247

Testing instructions

Use this branch of edx-platform: feat: Multiple updates to handle children upstream info [FC-0097] openedx-platform#37433
Create this structure in a library Section > Subsection > Unit > Text components.
Publish all the library
Add the Section from the library to a course.
Update a text component in the library. Publish the component.
In the course outline, click on the sync button. Verify that you can't see the alert.
Update the text component in the course (local update)
Click on the sync button. Verify that you can see the new alert.
Update another text component in the library. Publish the component.
In the course outline, click on the sync button. Verify that the alert is gone.

Other information

Merge this before the Ulmo cut

Best Practices Checklist

We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:

Any new files are using TypeScript (.ts, .tsx).
Deprecated propTypes, defaultProps, and injectIntl patterns are not used in any new or modified code.
Tests should use the helpers in src/testUtils.tsx (specifically initializeMocks)
Do not add new fields to the Redux state/store. Use React Context to share state among multiple components.
Use React Query to load data from REST APIs. See any apiHooks.ts in this repo for examples.
All new i18n messages in messages.ts files have a description for translators to use.
Imports avoid using ../. To import from parent folders, use @src, e.g. import { initializeMocks } from '@src/testUtils'; instead of from '../../../../testUtils'

openedx-webhooks · 2025-10-07T00:42:48Z

Thanks for the pull request, @ChrisChV!

This repository is currently maintained by @bradenmacdonald.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

codecov · 2025-10-07T00:52:44Z

Codecov Report

❌ Patch coverage is 97.05882% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.75%. Comparing base (4a26a86) to head (6d511fd).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/container-comparison/utils.ts	80.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #2516    +/-   ##
========================================
  Coverage   94.75%   94.75%            
========================================
  Files        1205     1206     +1     
  Lines       26949    26979    +30     
  Branches     6049     5907   -142     
========================================
+ Hits        25535    25564    +29     
- Misses       1344     1356    +12     
+ Partials       70       59    -11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

src/container-comparison/CompareContainersWidget.tsx

navinkarkera · 2025-10-08T07:16:51Z

src/container-comparison/CompareContainersWidget.tsx

+  const { data } = useCourseContainerChildren(downstreamBlockId, true);
+  let showLocalUpdateAlert = false;
+  let localUpdateAlertBlockName = '';
+
+  // Show this alert if the only change is a local override to a text component
+  if (!isReadyToSyncIndividually && data?.upstreamReadyToSyncChildrenInfo.length === 1
+        && data.upstreamReadyToSyncChildrenInfo[0].isModified
+        && data.upstreamReadyToSyncChildrenInfo[0].blockType === 'html') {
+    showLocalUpdateAlert = true;
+    localUpdateAlertBlockName = data.upstreamReadyToSyncChildrenInfo[0].name;
+  }
+


This section can be moved inside CompareContainersWidgetInner component and duplicate calls to useCourseContainerChildren api can be avoided. As we only want to fetch getUpstreamInfo at top level in diff preview, we can use parent.length==0 like so:

const { data, isError, error } = useCourseContainerChildren(downstreamBlockId, parent.length === 0);

@navinkarkera Thanks for the recommendation!
I decided not to put this in CompareContainersWidgetInner because if you enter a child, the alert would disappear. By keeping this call in CompareContainersWidget, the alert remains in the modal regardless of whether you navigate within the children. I will add comments about this. What do you think?

@ChrisChV Makes sense.

navinkarkera

@ChrisChV This looks good except for the issue with the logic of determining rename or content updates. I don't mind if you think we can fix it as part of another ticket.

navinkarkera · 2025-10-09T06:36:40Z

src/container-comparison/utils.ts

      let state: ContainerState | undefined;
      const displayName = oldVersion.upstreamLink.isModified ? oldVersion.name : newVersion.displayName;
      let originalName: string | undefined;
+      const isRenamed = displayName !== newVersion.displayName && displayName === oldVersion.name;


This logic doesn't work when the content is updated locally and the upstream display name is updated. isRenamed becomes true.

We probably need to differentiate between contentModified and rename in the backend or send downstream_customized field to the frontend and use it here.

I think we can fix it in a separate ticket; we are out of budget on this ticket. I will add a coment about that

navinkarkera · 2025-10-09T06:39:40Z

src/container-comparison/CompareContainersWidget.tsx

+  const { data } = useCourseContainerChildren(downstreamBlockId, true);
+  let showLocalUpdateAlert = false;
+  let localUpdateAlertBlockName = '';
+
+  // Show this alert if the only change is a local override to a text component
+  if (!isReadyToSyncIndividually && data?.upstreamReadyToSyncChildrenInfo.length === 1
+        && data.upstreamReadyToSyncChildrenInfo[0].isModified
+        && data.upstreamReadyToSyncChildrenInfo[0].blockType === 'html') {
+    showLocalUpdateAlert = true;
+    localUpdateAlertBlockName = data.upstreamReadyToSyncChildrenInfo[0].name;
+  }
+


@ChrisChV Makes sense.

src/container-comparison/CompareContainersWidget.tsx

navinkarkera · 2025-10-09T06:47:50Z

src/container-comparison/CompareContainersWidget.tsx

+  // We decided not to put this in `CompareContainersWidgetInner` because if you enter a child,
+  // the alert would disappear. By keeping this call in CompareContainersWidget,
+  // the alert remains in the modal regardless of whether you navigate within the children.
+  if (!isReadyToSyncIndividually && data?.upstreamReadyToSyncChildrenInfo.length === 1


@ChrisChV I missed this before, but this will only work if a single modified text component is ready to sync. I think we need to show this alert even if there are multiple text component (all locally modified) ready to sync no?

In the issue I have not seen mentioned in plural, it is always in singular, but I have asked the question: #2438 (comment)

Updated to support multiple text components with local overrides: 1afbcda

navinkarkera

@ChrisChV Great work 👍

I tested this: (Verified alert behaviour)
I read through the code
I checked for accessibility issues
Includes documentation

bradenmacdonald

This is a lot of special-case code, so I hope we can remove or simplify it in the future. But the code itself is fine, my concern is more with the spec.

Just a couple nits that are optional.

src/container-comparison/CompareContainersWidget.tsx

bradenmacdonald · 2025-10-15T21:36:19Z

Thanks; go ahead and merge anytime.

- Shows an alert in container sync if the only change is a local override to a text component [FC-0097] (openedx#2516) - Implements the alert described in openedx#2438 (comment)

ChrisChV added 2 commits October 6, 2025 16:44

feat: Add locally content updated state

b889eb7

feat: Add Alert when there is only one text component with local changes

b09fc50

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 7, 2025

openedx-webhooks added this to Contributions Oct 7, 2025

github-project-automation bot moved this to Needs Triage in Contributions Oct 7, 2025

ChrisChV changed the title ~~feat: Shows an alert in container sync if the only change is a local override to a text component []~~ feat: Shows an alert in container sync if the only change is a local override to a text component [FC-0097] Oct 7, 2025

ChrisChV mentioned this pull request Oct 7, 2025

feat: Multiple updates to handle children upstream info [FC-0097] openedx/openedx-platform#37433

Merged

Merge branch 'master' into chris/FAL-4247-child-text-override

fa4a2f0

ChrisChV mentioned this pull request Oct 7, 2025

Child text override sync handling #2438

Closed

test: Add tests to cover the new code

9295bd3

navinkarkera suggested changes Oct 8, 2025

View reviewed changes

style: Add comments

780e4ce

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Oct 8, 2025

mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions Oct 8, 2025

navinkarkera reviewed Oct 9, 2025

View reviewed changes

ChrisChV added 2 commits October 9, 2025 18:34

style: Nits on the code

de3aba3

feat: Add alert for multiple text components with local overrides

1afbcda

ChrisChV requested a review from navinkarkera October 11, 2025 14:19

navinkarkera approved these changes Oct 13, 2025

View reviewed changes

ChrisChV requested a review from bradenmacdonald October 13, 2025 15:21

bradenmacdonald approved these changes Oct 14, 2025

View reviewed changes

src/container-comparison/CompareContainersWidget.tsx Outdated Show resolved Hide resolved

src/container-comparison/CompareContainersWidget.tsx Outdated Show resolved Hide resolved

ChrisChV added 2 commits October 15, 2025 15:37

Merge branch 'master' into chris/FAL-4247-child-text-override

c0bd9e3

style: Nits on the code

6d511fd

ChrisChV merged commit 311bef6 into openedx:master Oct 16, 2025
7 checks passed

github-project-automation bot moved this from In Eng Review to Done in Contributions Oct 16, 2025

ChrisChV deleted the chris/FAL-4247-child-text-override branch October 16, 2025 00:18

Conversation

ChrisChV commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Other information

Best Practices Checklist

Uh oh!

openedx-webhooks commented Oct 7, 2025

Uh oh!

codecov bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navinkarkera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navinkarkera left a comment

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bradenmacdonald commented Oct 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ChrisChV commented Oct 7, 2025 •

edited

Loading

codecov bot commented Oct 7, 2025 •

edited

Loading